lnd+walletunlocker: make unlock/init operations synchronous #4349
Conversation
|
As is, this makes unlock/init fully synchronous. If we want to leave it as async, then a goroutine can be added in the unlocker service to release the mutex in the background after the operation is complete. |
| @@ -313,6 +342,9 @@ func (u *UnlockerService) InitWallet(ctx context.Context, | |||
| func (u *UnlockerService) UnlockWallet(ctx context.Context, | |||
| in *lnrpc.UnlockWalletRequest) (*lnrpc.UnlockWalletResponse, error) { | |||
|
|
|||
| u.Lock() | |||
| defer u.Unlock() | |||
There was a problem hiding this comment.
I think we may also need another signal/bool at the UnlockerService level that indicates the wallet has been initialized/unlocked so that we can check it here and everywhere else, otherwise it seems like we still risk attempting to open the wallet twice.
There was a problem hiding this comment.
What do you mean? Push things down even further? With where things are atm, we won't return back to the caller until the wallet has been fully initialized. However, for unlock we return a bit earlier once we have all the credentials. In my testing, the concurrent init was was ended up tripping things up.
For unlock, things only become "fully finalized" once we create the chain control.
There was a problem hiding this comment.
Actually, as soon as this method returns, unlocking isn't even possible since the listener service only has a lifetime of this call.
In this commit, we fix a deadlock that can happen if a user attempts to init then rapidly unlock a wallet right after. In my profiles, it seems the lnd gets caught up on the bbolt flock, which deadlocks the entire process. We fix this issue by making the Init/Unlock calls now fully synchronous. Only a single outstanding request can exist across the entire wallet unlocker service now. Fixes lightningnetwork#4330. Fixes lightningnetwork#3631.
Roasbeef
force-pushed
the
sync-wallet-unlock
branch
from
3b4f025
to
d533851
Compare
| @@ -472,7 +504,20 @@ func TestChangeWalletPassword(t *testing.T) { | |||
| t.Fatalf("expected to receive password %x, got %x", | |||
| testPassword, unlockMsg.Passphrase) | |||
| } | |||
|
|
|||
| // We'll now close the done channel to unlock the service to be | |||
| // able to accept another request. | |||
There was a problem hiding this comment.
from your comment on the pr it seems the unlocker can only process one request, but several comments in the code refer to processing more requests. is it the case that the wallet unlocker could process more requests, but we only allow one due to the defer cancel() in waitForWalletPassword?
| @@ -302,7 +323,19 @@ func (u *UnlockerService) InitWallet(ctx context.Context, | |||
| initMsg.ChanBackups = *chansToRestore | |||
| } | |||
|
|
|||
| u.InitMsgs <- initMsg | |||
| select { | |||
There was a problem hiding this comment.
add helper to avoid having 3 copies of the same code?
There was a problem hiding this comment.
guess there are really two versions, one for init and one for unlock...
| u.UnlockMsgs <- walletUnlockMsg | ||
| select { | ||
| case u.UnlockMsgs <- walletUnlockMsg: | ||
| case <-u.quitChan: |
There was a problem hiding this comment.
Ah I see, the main shutdown chan is passed to it.
| @@ -1124,6 +1130,13 @@ func waitForWalletPassword(cfg *Config, restEndpoints []net.Addr, | |||
| // The wallet has already been created in the past, and is simply being | |||
| // unlocked. So we'll just return these passphrases. | |||
| case unlockMsg := <-pwService.UnlockMsgs: | |||
|
|
|||
There was a problem hiding this comment.
I was still able to reproduce the issue with the current changes. The issue doesn't seem to be related to the init call being asynchronous, but rather with the UnlockerService having a queued UnlockWallet call that it doesn't cancel after InitWallet has been called. If we had a UnlockerService.Quit method, we could call it here and check that it's been closed after the lock has been acquired at the UnlockerService level.
|
Replaced by #4985 |
In this commit, we fix a deadlock that can happen if a user attempts to
init then rapidly unlock a wallet right after. In my profiles, it seems
the lnd gets caught up on the bbolt flock, which deadlocks the entire
process. We fix this issue by making the Init/Unlock calls now fully
synchronous. Only a single outstanding request can exist across the
entire wallet unlocker service now.
Fixes #4340.
Fixes #3631.